opcua_plugin/node_attributes.go (4)

11-19: Consider making usage examples or docstrings clearer.
The AttributeHandler struct is well laid out, but it might be helpful to include usage comments or docstrings in its fields to guide future maintainers and reduce confusion about how each flag (e.g., affectsNodeClass) should be used in context.

---

21-59: Consider returning user-friendly errors or status.
handleAttributeStatus returns an unwrapped attr.Status or a generic error message in some cases. Consider returning more contextual error messages if attr.Status is not ua.StatusOK to provide additional information, especially for debugging or logging.

---

79-161: Consider simplifying high-level flow in processNodeAttributes.
While the function is clear in purpose, the repetitive pattern of calling handleAttributeStatus for each attribute could be further streamlined using shared logic. If performance isn’t the highest priority here, a small refactor or a loop-based approach might reduce boilerplate.

---

163-183: Add coverage for unknown DataType cases.
The fallback conversion in getDataTypeString returns a string with ns=0 and the raw typeID. Consider logging or returning a specific string to highlight unknown types, so downstream processes can differentiate truly unknown types from ones that map to standard primitives.

opcua_plugin/browse.go (1)

137-138: Potential micro-optimization.
Rather than manually checking path != "" and concatenating with a dot, you could reuse the existing join function further below for path assembly, keeping path concatenation consistent and reducing possible logic duplication.

- newPath = path + "." + newPath
+ newPath = join(path, newPath)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

opcua_plugin/node_attributes.go (4)

11-19: Consider enhancing field documentation.

While the struct is well-documented, consider adding type information to the field comments for better clarity. For example:

// handleOK is the function that handles the OK status
// Type: func(value *ua.Variant) error

---

21-58: Improve error messages and code structure in handleAttributeStatus.

Consider these improvements:

1. Use more descriptive error messages by including the attribute name in all error cases
2. Implement early returns to reduce nesting and improve readability

 func handleAttributeStatus(
     name string,
     attr *ua.DataValue,
     def *NodeDef,
     path string,
     logger Logger,
     handler AttributeHandler,
 ) error {
     if attr == nil {
-        return fmt.Errorf("attribute is nil")
+        return fmt.Errorf("attribute %s is nil", name)
     }

     if attr.Status == ua.StatusOK {
         if attr.Value == nil && handler.requiresValue {
             return fmt.Errorf("attribute value is nil for %s", name)
         }
         return handleOKStatus(attr.Value, handler)
     }

     if attr.Status == ua.StatusBadSecurityModeInsufficient {
-        return errors.New("insufficient security mode")
+        return fmt.Errorf("insufficient security mode for attribute %s", name)
     }

     if attr.Status == ua.StatusBadAttributeIDInvalid {
         if handler.ignoreInvalidAttr {
             return nil
         }
-        return fmt.Errorf("invalid attribute ID")
+        return fmt.Errorf("invalid attribute ID for %s", name)
     }

     if attr.Status == ua.StatusBadNotReadable {
         handleNotReadable(def, handler, path, logger)
         return nil
     }

     return fmt.Errorf("unexpected status %v for attribute %s", attr.Status, name)

---

158-178: Consider enhancing dataTypeMap coverage.

1. Consider adding support for additional OPC UA data types:

   • Int64/UInt64
   • ByteString
   • XmlElement
   • LocalizedText
   • QualifiedName

2. Add validation for the fallback format in getDataTypeString:

 func getDataTypeString(typeID uint32) string {
     if dtype, ok := dataTypeMap[typeID]; ok {
         return dtype
     }
+    if typeID == 0 {
+        return "unknown"
+    }
     return fmt.Sprintf("ns=%d;i=%d", 0, typeID)
 }

---

1-178: Well-structured extraction of node attribute handling.

The code successfully achieves its goal of reducing cyclomatic complexity by extracting node attribute handling into a separate, well-organized module. The implementation follows Go best practices and provides comprehensive error handling.

The AttributeHandler pattern provides good flexibility and separation of concerns, making the code more maintainable and testable.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

opcua_plugin/node_attributes.go (2)

20-57: Consider using constants and early returns for better readability.

The error handling is thorough, but the code could be more maintainable with these improvements:

+// OPC UA status constants for better readability
+const (
+    statusOK = ua.StatusOK
+    statusBadSecurityMode = ua.StatusBadSecurityModeInsufficient
+    statusBadAttributeID = ua.StatusBadAttributeIDInvalid
+    statusBadNotReadable = ua.StatusBadNotReadable
+)

 func handleAttributeStatus(
     name string,
     attr *ua.DataValue,
     def *NodeDef,
     path string,
     logger Logger,
     handler AttributeHandler,
 ) error {
     if attr == nil {
         return fmt.Errorf("attribute is nil for node: %s and attribute: %s", def.NodeID, name)
     }
 
-    if attr.Status == ua.StatusOK {
-        if attr.Value == nil && handler.requiresValue {
-            return fmt.Errorf("attribute value is nil for node: %s and attribute: %s", def.NodeID, name)
-        }
-        return handleOKStatus(attr.Value, handler)
+    switch attr.Status {
+    case statusOK:
+        if attr.Value == nil && handler.requiresValue {
+            return fmt.Errorf("attribute value is nil for node: %s and attribute: %s", def.NodeID, name)
+        }
+        return handleOKStatus(attr.Value, handler)
+    case statusBadSecurityMode:
+        return fmt.Errorf("got insufficient security mode for node: %s and attribute: %s", def.NodeID, name)
+    case statusBadAttributeID:
+        if handler.ignoreInvalidAttr {
+            return nil
+        }
+        return fmt.Errorf("invalid attribute ID for node: %s and attribute: %s", def.NodeID, name)
+    case statusBadNotReadable:
+        handleNotReadable(def, handler, path, logger)
+        return nil
     }
-
-    if attr.Status == ua.StatusBadSecurityModeInsufficient {
-        return fmt.Errorf("got insufficient security mode for node: %s and attribute: %s", def.NodeID, name)
-    }
-
-    if attr.Status == ua.StatusBadAttributeIDInvalid {
-        if handler.ignoreInvalidAttr {
-            return nil
-        }
-        return fmt.Errorf("invalid attribute ID for node: %s and attribute: %s", def.NodeID, name)
-    }
-
-    if attr.Status == ua.StatusBadNotReadable {
-        handleNotReadable(def, handler, path, logger)
-        return nil
-    }
 
     return attr.Status
 }

---

157-177: Add documentation for the default format.

Consider adding a comment explaining the format used when a type ID is not found in the map.

 func getDataTypeString(typeID uint32) string {
     if dtype, ok := dataTypeMap[typeID]; ok {
         return dtype
     }
+    // Return the OPC UA node ID format (ns=0;i=<typeID>) for unknown types
     return fmt.Sprintf("ns=%d;i=%d", 0, typeID)
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro